-
Notifications
You must be signed in to change notification settings - Fork 48
Add auth{n,z} USDT probes to Nexus #8566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Partial fix for #8424
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! This is a nice improvement.
A few thoughts:
- It'd be nice to have some example output somewhere. Docs on tracing Nexus would be nice but would be much more work. Maybe just drop some example output in the D script itself?
- For authn, I imagine we might want to know what actor tried to (or did) authenticate. We might already have this in the result? (sample output would show). I'm torn about whether we'd also want to provide the token that's being authenticated. Since it's sensitive (so there's downside to including it), it's probably not worth including until/unless we have a use case.
- If we have other "nexus" provider probes, do these just show up alongside all of those? So that as far as consumers are concerned, they just see one "Nexus" provider with these probes and the other ones? That seems ideal but I didn't realize that was possible.
Will do.
The authentication result does already contain the actor, if the authentication was successful. If the authentication failed, we include the reason. That sometimes includes the actor, but not always. Is that enough, or should we modify things to always include the actor (or attempted actor)?
That's right. You can rename the provider using |
That's great. If we need more, we can always iterate. |
I've added example output to both D scripts in a68d33e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That output helps me realize: we're really going to want the request id here, aren't we? I'm imagining the major use cases for this are going to involve correlating authn/authz activity with specific requests. Is that easy to plumb through?
We can include that pretty easily for the authn probes, but the authz probes are more difficult. Those might not even have a request ID, such as during sagas or background tasks. We could include it when we have it, and some other, random-ish string when we don't? |
Yeah, that makes sense. Maybe NULL if there is none? |
Yeah, that could work. This is a bigger change, but we might consider putting the request ID into the The main justification for this is that almost every authz check calls |
I'm also surprised it's not already there! I agree it should be -- that's exactly the sort of thing that's intended to be there, and exactly for reasons like this. |
Agreed. I'm going to put that up in a separate PR, and then merge or rebase this on top of that. |
- Add request ID to all auth probes - Print request ID in D scripts and include in example output
I've added the request ID where possible in 6cfdd3d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Partial fix for #8424